fix(cua-driver/linux): retry root-only AT-SPI tree on cold Qt6 launch (#1927) - #1998
Merged
Merged
Conversation
…#1927) On Linux, get_window_state against a Qt6 app launched as the first/only accessibility client can return just the root window (element_count=1, empty tree) — the Qt6 AT-SPI bridge registers lazily and org.a11y.atspi.Registry hasn't finished enumerating the app when the first walk runs. The full tree appears only once the registry is already active. walk_tree_bounded returned early as soon as native::walk_tree produced non-empty markdown, and a root-only tree IS non-empty, so the degenerate result was returned. Retry the native walk up to 4 times with a 150ms backoff while the tree is root-only (nodes.len() <= 1), accepting any real tree immediately and the root-only result on the final attempt. Adds latency only on the cold-start path; a populated window returns on the first attempt. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KMXCW4M5uK1HRGjjH4wueZ
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
📝 WalkthroughWalkthrough
ChangesAT-SPI Cold-Launch Retry
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/cua-driver/rust/crates/platform-linux/src/atspi/mod.rs`:
- Around line 67-79: The retry heuristic in the walk_tree_bounded loop treats
nodes.len() <= 1 as a cold-start symptom unconditionally, causing unnecessary
sleep delays even when callers intentionally request tiny traversals via
max_elements=1 or max_depth=1. Modify the condition on line 73 that checks
nodes.len() > 1 to also account for the max_elements and max_depth parameters,
so that single-node results are accepted immediately when they match the
caller's intentional constraints, rather than forcing the retry loop to
completion. Only treat minimal results as cold-start symptoms when they're
genuinely unexpected given the traversal caps.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 83c11923-106e-4c48-a4f9-52ee3a3ee4a1
📒 Files selected for processing (1)
libs/cua-driver/rust/crates/platform-linux/src/atspi/mod.rs
Comment on lines
+67
to
+79
| for attempt in 0..MAX_ATTEMPTS { | ||
| if let Ok(Some((raw_md, nodes))) = native::walk_tree_bounded(pid, max_elements, max_depth) { | ||
| // `nodes.len() <= 1` == only the root window resolved: the | ||
| // cold-registry symptom. Accept any real tree immediately; only | ||
| // keep waiting on the degenerate case, and accept it anyway on the | ||
| // final attempt rather than discarding a (minimal) valid result. | ||
| if !raw_md.is_empty() && (nodes.len() > 1 || attempt == MAX_ATTEMPTS - 1) { | ||
| let md = if let Some(q) = query { filter_tree(&raw_md, q) } else { raw_md }; | ||
| return AtspiTreeResult { tree_markdown: md, nodes }; | ||
| } | ||
| } | ||
| if attempt < MAX_ATTEMPTS - 1 { | ||
| std::thread::sleep(std::time::Duration::from_millis(150)); |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Retry heuristic should be cap-aware to avoid forced backoff on intentionally tiny trees.
Line 73 treats nodes.len() <= 1 as a cold-start symptom unconditionally. But callers can intentionally request tiny traversals (max_elements=1 / max_depth=1), where a single-node result is expected; this path then always incurs 3 sleeps (~450ms) before returning.
Proposed fix
pub fn walk_tree_bounded(
pid: u32,
xid: u64,
query: Option<&str>,
max_elements: Option<usize>,
max_depth: Option<usize>,
) -> AtspiTreeResult {
const MAX_ATTEMPTS: usize = 4;
+ const BACKOFF_MS: u64 = 150;
+ let retry_root_only = max_elements.map_or(true, |m| m > 1)
+ && max_depth.map_or(true, |d| d > 1);
+
for attempt in 0..MAX_ATTEMPTS {
if let Ok(Some((raw_md, nodes))) = native::walk_tree_bounded(pid, max_elements, max_depth) {
- if !raw_md.is_empty() && (nodes.len() > 1 || attempt == MAX_ATTEMPTS - 1) {
+ let root_only = nodes.len() <= 1;
+ let should_retry = retry_root_only && root_only && attempt < MAX_ATTEMPTS - 1;
+ if !raw_md.is_empty() && !should_retry {
let md = if let Some(q) = query { filter_tree(&raw_md, q) } else { raw_md };
return AtspiTreeResult { tree_markdown: md, nodes };
}
}
if attempt < MAX_ATTEMPTS - 1 {
- std::thread::sleep(std::time::Duration::from_millis(150));
+ std::thread::sleep(std::time::Duration::from_millis(BACKOFF_MS));
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libs/cua-driver/rust/crates/platform-linux/src/atspi/mod.rs` around lines 67
- 79, The retry heuristic in the walk_tree_bounded loop treats nodes.len() <= 1
as a cold-start symptom unconditionally, causing unnecessary sleep delays even
when callers intentionally request tiny traversals via max_elements=1 or
max_depth=1. Modify the condition on line 73 that checks nodes.len() > 1 to also
account for the max_elements and max_depth parameters, so that single-node
results are accepted immediately when they match the caller's intentional
constraints, rather than forcing the retry loop to completion. Only treat
minimal results as cold-start symptoms when they're genuinely unexpected given
the traversal caps.
Contributor
Linux visual regression artifactsMatrix jobs now run independently. Download visual artifacts from this workflow run.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
On Linux,
get_window_stateagainst a Qt6 app launched as the first / only accessibility client sometimes returns just the root window (element_count=1, empty tree). The full tree (e.g. 155 elements) appears only onceorg.a11y.atspi.Registryis already active. Qt6 has the AT-SPI bridge built in, but registration is timing-sensitive on a cold launch. Fixes #1927.Root cause
atspi::walk_tree_boundedreturned as soon asnative::walk_treeproduced non-empty markdown — and a root-only tree (the window node with no children) is non-empty, so the degenerate cold-start result was accepted and returned.Fix
Retry the native walk up to 4 times with a 150 ms backoff while the tree is root-only (
nodes.len() <= 1), giving the registry time to finish enumerating the app. Any tree with real content is accepted on the first attempt; the root-only result is still accepted on the final attempt rather than discarded (and the existing X11-properties fallback remains for the truly-empty case). Latency is added only on the cold-start path — a populated window returns immediately.This implements the issue's "retry/wait briefly for the tree to populate when it comes back with only the root" direction. (A more proactive option — having the daemon pre-activate the registry /
toolkit-accessibilityat startup — is left as a possible follow-up.)Verification
cargo build -p platform-linuxrecompiled green at commitb37d7cc.🤖 Generated with Claude Code
Summary by CodeRabbit